Skip to content

Fix sorting by columns with nil values#1840

Draft
markusgust wants to merge 1 commit intomainfrom
fix/sort-nil-column-values
Draft

Fix sorting by columns with nil values#1840
markusgust wants to merge 1 commit intomainfrom
fix/sort-nil-column-values

Conversation

@markusgust
Copy link
Copy Markdown
Contributor

Summary

  • Fix crash when sorting by columns that can have nil values (e.g. policy on exchanges/queues)
  • Scan for first non-nil value to determine sort type, use safe casts with fallbacks
  • Fix dig() to return nil instead of 0 for missing values to avoid type confusion
  • Extract sort_by_key! helper to keep cyclomatic complexity within limits

Test plan

  • Sort exchanges by policy column in management UI (mix of exchanges with and without policies)
  • Sort queues by policy column
  • Verify existing sort functionality (by name, type, message rate) still works
  • Verify reverse sort works on nullable columns

🤖 Generated with Claude Code

The sort method crashed when sorting by columns that can have nil values
(e.g. policy on exchanges/queues). Fix by scanning for the first non-nil
value to determine the sort type, and using safe casts with fallbacks
during sorting. Also fix dig() to return nil instead of 0 for missing
values to avoid type confusion.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Apr 7, 2026

PR Review

Bug: when Nil branch is unreachable — all-nil column causes bad_request error

File: src/lavinmq/http/controller.cr:79-82

sample_value = sorted_items.each do |item|
  v = dig(item, sort_by)
  break v unless v.nil?
end

In Crystal, Array#each returns self (the array) when the block completes without break. So when all values are nil, sample_value is assigned the array itself, not nil. This means the when Nil branch on line 90 is dead code — instead, the else branch fires and returns a spurious bad_request error like "Can't sort on type Array(...)".

Fix: Use a separate variable so the nil case is handled correctly:

sample_value = nil
sorted_items.each do |item|
  v = dig(item, sort_by)
  if !v.nil?
    sample_value = v
    break
  end
end

Or more idiomatically, something like:

sample_value = sorted_items.find { |item| !dig(item, sort_by).nil? }.try { |item| dig(item, sort_by) }

The spec doesn't cover the all-nil case (it always creates at least one exchange with a policy), so this bug would not be caught by the current test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant